-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New/validate types #467
New/validate types #467
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks nice! Just some nits/conveniences
src/types/custom.rs
Outdated
SmolStr::new_inline("MyRsrc"), | ||
TypeBound::Eq, | ||
); | ||
|
||
pub(crate) const COPYABLE_CUST: CustomType = CustomType::new_simple( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could replace this with FLOAT64_T?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it is actually used to test custom types later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd very happily move this back, or indeed remove it, if it let me use prelude_registry
everywhere rather than needing test_registry
as well...but FLOAT64_T is in arithmetic which is outside prelude, right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so it is....could be moved....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found at least one place calling COPYABLE_T "float" and using it as if it were such, so I replaced that one with FLOAT64_TYPE....then if I'd done one then I've now done all the others...RIP COPYABLE_T.
I've kept test_registry
(==prelude + float_types), but could do the move yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave moving until someone adds a load of rotation operations that use angles...
src/extension.rs
Outdated
let mut reg = Self::new(); | ||
for ext in value.into_iter() { | ||
let prev = reg.0.insert(ext.name.clone(), ext); | ||
assert!(prev.is_none()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be a try from, or at least add a message to the assert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done the latter, try-from leads to a conflicting instance error, or else one about uncovered type params. (There's blanket TryFrom = From
+ Infallible
thrown into the mix.)
// And check they fit into the TypeParams declared by the TypeDef | ||
let ex = extension_registry.get(&self.extension); | ||
// Even if OpDef's (+binaries) are not available, the part of the Extension definition | ||
// describing the TypeDefs can easily be passed around (serialized), so should be available. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ss2165 note this is a significant policy commitment!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a commitment in extension loading/unloading you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, or at least, that when we serialize a Hugr, we should serialize out this part of all Extensions with it
TypeArg::Type(ty) => ty.validate(extension_registry), | ||
TypeArg::BoundedNat(_) => Ok(()), | ||
TypeArg::Opaque(custarg) => { | ||
// We could also add a facility to Extension to validate that the constant *value* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm inclined to make an issue for this...?
896fba3
to
cad92b0
Compare
} | ||
|
||
#[fixture] | ||
pub(crate) fn simple_dfg_hugr() -> Hugr { | ||
let dfg_builder = | ||
DFGBuilder::new(FunctionType::new(type_row![BIT], type_row![BIT])).unwrap(); | ||
let [i1] = dfg_builder.input_wires_arr(); | ||
dfg_builder.finish_hugr_with_outputs([i1]).unwrap() | ||
dfg_builder.finish_prelude_hugr_with_outputs([i1]).unwrap() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
method names are getting unwieldy, one for another time
BREAKING CHANGE(1): Hugr::validate() and the various finish_hugr[_with_outputs] Builder methods require an ExtensionRegistry, e.g. &EMPTY_REG or &prelude_registry(). Note new Builder methods finish_prelude_hugr[_with_outputs]
BREAKING CHANGE(2): crate::types::test::{EQ_T, COPYABLE_T, ANY_T} are gone - try USIZE_T, FLOAT64_TYPE, QB_T from prelude and/or arithmetic extensions.